Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix CngKey.KeySize for keys in the Platform Crypto Provider #77809

Merged
merged 11 commits into from
Jan 4, 2023

Conversation

vcsjones
Copy link
Member

@vcsjones vcsjones commented Nov 2, 2022

Contributes to #75971.
Contributes to #71009.

The MicrosoftPlatformCryptoProvider does not correctly report the KeySize for CngKey (and by extension, ECDsaCng and ECDiffieHellmanCng key size properties) and instead always returns zero.

The ECCCurveName CNG property does appear to be properly reported for those keys, so when we find ourselves in this circumstance, use the curve name if available to properly report the key size.

@ghost
Copy link

ghost commented Nov 2, 2022

Tagging subscribers to this area: @dotnet/area-system-security, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

Contributes to #75971.

The MicrosoftPlatformCryptoProvider does not correctly report the KeySize for CngKey (and by extension, ECDsaCng and ECDiffieHellmanCng key size properties) and instead always returns zero.

The ECCCurveName CNG property does appear to be properly reported for those keys, so when we find ourselves in this circumstance, use the curve name if available to properly report the key size.

Author: vcsjones
Assignees: vcsjones
Labels:

area-System.Security

Milestone: -

@vcsjones
Copy link
Member Author

vcsjones commented Nov 2, 2022

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Co-authored-by: Stephen Toub <stoub@microsoft.com>
@vcsjones
Copy link
Member Author

vcsjones commented Nov 3, 2022

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@vcsjones
Copy link
Member Author

vcsjones commented Nov 3, 2022

I am quite perplexed by the outerloop test results. Every test suite seems to be crashing on multiple operating systems? These changes are isolated to Windows, so I would be a little surprised if these changes are breaking OSX-outerloop.

@vcsjones
Copy link
Member Author

vcsjones commented Nov 7, 2022

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@vcsjones
Copy link
Member Author

vcsjones commented Nov 7, 2022

One more try to see if it was an infrastructure issue.

@vcsjones
Copy link
Member Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@vcsjones
Copy link
Member Author

@bartonjs I've been blocked on getting a meaningful outerloop run here for a while. Am I using the right pipeline? Thoughts on how to proceed?

@bartonjs
Copy link
Member

I think you're hitting #76755... which, AFAICT, means that all outerloop runs are broken.

I've asked for contrary evidence (such as a variant config that would run the test, but works), but since I haven't received any I decided to say this much.

@vcsjones
Copy link
Member Author

Okay. I guess this PR can just hang out for a while. Getting the PCP working with CNG will need a functioning outerloop.

Merge ms/main into vcsjones/cng-key-size
@vcsjones
Copy link
Member Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@vcsjones
Copy link
Member Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@build-analysis build-analysis bot mentioned this pull request Dec 21, 2022
@vcsjones
Copy link
Member Author

So it looks like Outerloop is working again, and the failures appear unrelated. However, the new tests are not running in CI because it appears that the CI machines do not have a functional TPM.

Condition(s) not met: "PlatformCryptoProviderFunctional"

@bartonjs I'm going to hold off on merging this until you are back and may have additional input on if you want to merge this, or find a different approach to testing, if at all possible. Otherwise, the only way we'll catch regressions is running outerloop locally.

@bartonjs bartonjs merged commit de0ba02 into dotnet:main Jan 4, 2023
@vcsjones vcsjones deleted the cng-key-size branch January 4, 2023 18:01
@ghost ghost locked as resolved and limited conversation to collaborators Feb 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants